Skip to content

nixos: use uniq in the type of system.build#155963

Merged
roberth merged 1 commit intoNixOS:masterfrom
ncfavier:system-build-type
Jan 20, 2022
Merged

nixos: use uniq in the type of system.build#155963
roberth merged 1 commit intoNixOS:masterfrom
ncfavier:system-build-type

Conversation

@ncfavier
Copy link
Member

@ncfavier ncfavier commented Jan 20, 2022 •
edited
Loading

unspecified will happily concatenate strings together from two unrelated modules, causing spurious errors (see #155925).

Ideally we could make system.build a submodule with a few distinguished options like installBootLoader, but I don't think this is necessary right now.

I've tested this by running a vm with build-vm-with-bootloader.

@ncfavier ncfavier requested a review from dasJ as a code owner January 20, 2022 22:13
@ncfavier ncfavier requested review from roberth and removed request for dasJ January 20, 2022 22:13
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jan 20, 2022
@ncfavier ncfavier requested a review from dasJ January 20, 2022 22:14
@roberth
Copy link
Member

roberth commented Jan 20, 2022

I didn't know that it did that, but anything also merges, deeply even. How about uniq unspecified?

Regarding the submodule, see blocked pr #155883.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jan 20, 2022
@ncfavier
Copy link
Member Author

ncfavier commented Jan 20, 2022

Sounds good, I looked at all definitions of system.build.* and couldn't find anything that relies on merging.

@ncfavier
nixos: use uniq in the type of system.build
1147d72 
`unspecified` will happily concatenate strings together from two
unrelated modules, causing spurious errors (see NixOS#155925).
@ncfavier ncfavier changed the title nixos: use anything instead of unspecified in the type of system.build nixos: use uniq in the type of system.build Jan 20, 2022
@ncfavier
Copy link
Member Author

ncfavier commented Jan 20, 2022

@ofborg test grub systemd-boot login

@roberth
Copy link
Member

roberth commented Jan 20, 2022 •
edited
Loading

Before attrsOf unspecified, it was attrs, which would replace attributes arbitrarily in case of duplicates; arbitrary being determined by the module import order and module system internals. So uniq will only find pre-existing problems.

@roberth
Copy link
Member

roberth commented Jan 20, 2022

OfBorg evaluates fine. The error on arm is caused by a build failure after eval, caused by a full disk.

I hope not too many collisions exist out there, because they're about to be discovered -- well, except for the fraction of users that turned away because they couldn't figure out why their config or module didn't apply because of the silent collisions. Sad.

Let's fix this problem for current and future configs. Merging.

@roberth roberth merged commit c4525c8 into NixOS:master Jan 20, 2022
@ncfavier ncfavier deleted the system-build-type branch January 20, 2022 23:12
@rhoriguchi rhoriguchi mentioned this pull request Jan 23, 2022
Closed
@SFrijters SFrijters mentioned this pull request Jan 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roberth roberth roberth approved these changes

@dasJ dasJ Awaiting requested review from dasJ

Assignees

No one assigned

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ncfavier @roberth